Interactive Map For Experimental Deck #644
Conversation
There was a problem hiding this comment.
This looks amazing! There are a couple of minor issues that should be resolved and a couple of others that might be worth thinking about, but I think we should merge asap to allow people to play around with it!
CONTRIBUTING.md
Nitpicks:
I think it'd be better if we had full stops at the ends of bullet points, at least when these are long/full sentences (where there's a mix of short/long, I think we should default to long for consistency). AFAICT for short bullet points, we've been following the convention of having a full stop at the end of the list (last bullet point), until now, but I don't have a strong opinion either way. (Sorry — the lack of full stops was weirdly distracting when reading!)
I think we should expand the acronym IIFE (immediately invoked function expression).
_interactive_map_config.js
Why are the booleans strings in getUserConfig? Is it just to allow us to be liberal in what we accept (i.e. all of "True", true, "true", "TRue" etc. being accepted)?
Also, maybe if we want the config to be more easily adjustable, by the user, we should move (the equivalent of) getUserConfig into the note template (Country - Map [Experimental].html). (This could be by, say, passing configObj to some function defined in _interactive_map_config.js.) (I think it's easier for a typical user to change the note template from within Anki rather than opening _interactive_map_config.js in a text editor, but it might not be worth the refactoring.)
_jsvectormap.js
Where does _jsvectormap.js come from? (In that I know that it's from jsvectormap but how is it generated/which version is it (currently) based off/what licence is it under?)
The former is valuable for reproducibility/maintenance purposes (if you're busy someone else might need to adjust the maps, and besides you might forget things you did yourself (I know that I do :)) — this should probably go in CONTRIBUTING.md.
License-wise, we need this to satisfy the attribution requirements of the MIT license (probably should go at the end of LICENSE.md).
_world.js
Ideally, we'd have instructions for how to generate _world.js as well, but I guess that that might be trickier.
Tests
We'll need a test (in .github/workflows/integrity-check.yml) that checks that pipenv run brain_brew run recipes/source_to_anki_\[experimental\].yaml runs successfully.
Infrastructure
Infrastructure - as the existing source_to_anki.yaml recipe being already complex enough, the infrastructure part of the experimental deck creation was written as a separate configuration - source_to_anki_[experimental].yaml.
I think that in the medium-term this is probably the right choice. In the longer term, it might be worth combining the two, to avoid duplication, despite the complexity.
The point is reinforced by the fact that the deck version numbering does not match its counterparts
I don't think that the deck version numbering is affected by where the BrainBrew recipe is located, though.
Also, regarding numbering, I'm not sure whether or not we should have separate numbering for the experimental deck — it depends on the length of the transition into the main deck. I think we have two main options:
- Use the same numbering as for the main decks, and if we have more frequent releases for the experimental deck use something like 5.2.1, 5.2.2 etc.
- Use 0.x for the first several experimental deck releases (and optionally migrate to the main deck versioning eventually (or not if we fully integrate everything before then).
Map colours
The discrepancy between the colours in the interactive map and the static maps is unfortunate, though I can't think of a better approach.
Using red if the user clicks the wrong region and green if they click the correct one is the natural, obvious choice. Unfortunately (as you know), we currently use red as the main highlight colour in our maps (in line with Wikipedia's/Wikimedia's style of maps). This leads to three related issues:
- The change in style (old static maps vs. new interactive maps) for the fully interactive Country - Map cards might be very slightly confusing for current users (obviously not an issue for new users). (IMO minor — people can get used to this.)
- The difference in style for the "Country - Map" and "Map - Country" cards (the former uses the interactive maps and the latter the current static maps). (Also minor.)
- The difference between the "Country - Map" cards that are interactive on the back side and those that have a static map. Here, there's a risk that people will think (for the back-static cards) that because the highlight is in red, they had incorrectly clicked*. (We can mention this in the README or elsewhere, but most people won't read.)
* actually, in a very narrow sense they will have incorrectly clicked, since the correct region is not represented on the interactive map...
Possible solutions:
-
Globally switch to green as highlight colour (in the static land maps).
Relatively simple to do — we have access to the original SVG images.
However, I'd be _very) hesitant, since I'm not sure if it would look nice (the Wikimedia contributors from the Map Workshop chose the colour scheme for a reason and I generally trust their taste/choices).
-
Chose another equally-intuitive colour scheme where red is correct and ??? is incorrect. (No idea what...)
-
Use some other indicators of correct/incorrect (borders?? frames?? Again no idea that wouldn't look ugly...)
-
Switch back to using the "globe" icon for the notes where the back is static.
IMO not a great idea as it provides a hint to the answer and as you noted, the interactive map is useful anyway.
Unless we have good ideas for 2 or 3 I think we should leave the green/red as they are.
Minor annoyances/thoughts
-
The zoom resets (i.e. you abruptly zoom out and then zoom in) when switching to the back card.
-
In exchange for the interactive map we lose the "disputed borders/territories" information etc. (as we had already discussed in the original issue). Maybe (but probably not) we should have a way of also displaying the static image map. A toggle/checkbox of some sort to switch between the static and interactive maps would probably be not-too-difficult to implement, but it would be ugly and result in additional clutter, so probably not.
-
The Mercator projection (which I think is what's used now) isn't great. The Winkel Tripel that we use for the static maps might have been better, but it's also not great away from the central longitude (e.g. the Americas/Oceania for 15 °). Switching the central longitude depending on country would provide hints...
README.md
I think we should have some sort of description of the interactivity — particularly meaning of the colours (intuitive though they are) and the presence of the static fallback for dependent terrritories and the like — in either the README or the deck description (if we were to have a separate deck description for the experimental deck).
Naming
To avoid file-collisions (as unlikely as they are), I think we should prefix the _... files with _ug (e.g. _ug-world.js etc.)?
Note models
The UUID in src/note_models/Ultimate_Geography_[Experimental].yaml should be different from that in src/note_models/Ultimate_Geography_[Extended].yaml.
Squash vs. merge
No strong opinion, here. Your commits seem to be clean (we sometimes have contributions that don't, so squashing is useful), with extensive descriptions, so it might be worth keeping the history in the repo.
Agreed.
Almost. String property is used so that typos (e.g.
Absolutely agree that it is easier for the user. Moreover I haven't yet figured out the issue with media folder update on AnkiDroid - it seems that the media is not always updated when a file is changed. You need to remove the file from media folder, then force sync on Anki Desktop and only then sync your AnkiDroid. Thus the configuration change may not be easily updated on other from main devices. Funny enough the config was initially in the note template. But half through development I refactored it to a separate script as I expected you to be less inclined to accept a lot of stuff in the note template.
Though exactly this solution (not the whole idea of configuration in note template) is flawed. The configuration is loaded separately for both front and back sides. So with such option we'd need to write configuration twice in note template - for both card sides. And every time it would have to be updated twice which is fraught with troubles due to primitive human error. The way I did it (before moving configuration to media folder) is via
That's on me. Indeed it is essential to know the version and have attribution in
This will be done in the next release. Right now it is skipped consciously as the instructions will quickly become deprecated. Currently program uses pre-formed (taken from the web) SVG file that gets converted by an ad-hoc tool into JS object stored in
Good point.
I meant that right now experimental deck releases might be more frequent, so dividing setup into separate recipes makes sense to avoid generating regular and extended decks every time and vice versa
The thing is I am not sure what is your plan on when (and if) experimental deck becomes "stable" enough. If not, then still, it would probably be confusing to users when one deck variant has only version
I would not worry about this issue too much as the plan is to eventually provide interactive map for every region (including water bodies). So, assuming the plan succeeds, the issue of discrepancy on the back side of a card between "correct interactive" and "correct static" is completely eliminated. Although, the issue of discrepancy of coloring between front card side static map and back card side interactive map remains (albeit minor) I suggest releasing it "as is" right now and after some time (or after the complete switch of back card side to interactive map) to gather user feedback (e.g. via Google Forms or Reddit post) for this and possibly other features we have concerns about / are interested in developing further. The other option is to configure default setup so that both correctly and incorrectly selected regions are highlighted in red (i.e. static map red). And the user can make conscious decision to change configuration to highlight correctly selected region in green understanding the consequences (that is, possible confusion). Though this option assumes less benefit of the green highlight (thus disabled by default), than confusion, which might not be precise. IMO not optimal, we can go with the first solution and if the feedback of the majority is negative of this feature - only then employ this option. Tell me what you think of it in case some changes in the codebase are needed.
Agreed - changing static maps to green is both questionable and requires a bit of work.
These are partially the issues that motivated the suggestion of creation of separate jsvectormap library fork (in case original library maintainer not responding to PRs) - mentioned in one of the original issue's comments:
The work on them will be started in the order of their priority out of all the remaining items.
You mean Mercator projection's downside of size and distance distortion? If so, yes, it exists. I've seen in the library codebase options for other projections. Eventually will have look at it, but for now it is low on priority list.
Agreed, user guide as a separate section of
Why not have in both ;)
Great catch, haven't thought about it before.
I see that UUID resembles MD5 hash with hyphens in certain places. Would it be okay to set UUID of What about By the way, I am not sure whether the process of upgrading from regular/extended deck to experimental is straightforward. Do we need separate instruction list for it?
Good to know as commits are expected to be indeed helpful to understand the reasoning of the written logic. To summarize:Changes to be included in the current release (before this PR merge):
Changes to be included in the next release(s):
Please be free to remind me of any items I have missed / forgotten |
|
Thanks very much for the reply! (For brevity, I've mostly skipped the parts where I was just writing "I agree"/"Makes sense" with no further comment :))
That makes sense and is a great idea! (Making the parts of the code intended to be adjusted by the user less brittle.)
Could we just move the parts that are really intended to be configured (i.e. probably just
Thanks for explaining! (I suspect that one could probably work around this using Versions and releases
Ah, yes, you're right!
Yes, that's the intention! (With possibly a fully-static variant deck — i.e. the interactive deck would become the "main" extended deck and the current extended version would become a variant.)
I was more thinking about the potential longer-term case if we don't merge and we have both "content" changes (new notes/languages etc.), and changes to the interactive map, and users of the interactive deck might want to know off which "main" deck version the interactive version is based. However, we can worry about this in the future (switching from 0.x to 5.2 etc. is straightforward).
Yes, definitely, IMO we should ask for feedback asap.
I agree that having colour-coding of correctness of response is preferable.
Yes, exactly.
I expect that it should be relatively straightforward (a matter of choosing a projection when converting from GeoJSON to SVG), but I agree that it's low priority for now.
That would work (but so would any (semi-)random UUID)!
I think that they should stay the same as those in * if we do have (a) separate deck description(s) for the interactive deck, then these UUIDs should also be changed.
It's pretty much the same as for upgrading from standard to extended: i.e. for upgrading from standard to interactive you'll have to make sure to switch the order of cards (like for standard->extended), and for extended->interactive you won't need to do anything. (I've just tested extended->interactive, and it does match expectations.) However, some instructions might be useful — possibly a set of tables in the AUG Wiki, along the lines of: Standard -> InteractiveCards
where bold means that that value has to be adjusted. Notes... ? (This can wait for after the merge, though — the (expected) upgrade path from Extended to Interactive/Experimental is pretty much frictionless, so a guide isn't essential.)
Most people don't write helpful commit messages, so "expected" is too strong a word: rather "very gratefully received". :)
Yes, I agree!
Yeah, these can wait! User-testingOne slight issue (I've only now realised) in terms of user-testing is that BrainBrew/CrowdAnki/AUG intentionally work in the way that changing/switching note templates results in the user's existing notes and cards being updated to the new templates, including the user's progress. For actually upgrading (as opposed to testing), this is very much a good thing, since the user likely doesn't want to start from scratch. However, for testing this unfortunately means that the most obvious approach of importing into one's current Anki profile, testing and deleting the newly imported cards, does not work. Testing in a new profile obviously does work. However, this is tricky when one wants to test on mobile — and we do want third-party mobile testing, for iOS in particular! Upgrading and then downgrading (i.e. import interactive and then import extended) also works (in that there's no data loss/progress loss, unless the user had edited their notes/note templates, in which case they do lose their edit (but they'd lose them anyway when upgrading)). However, this is finicky (multiple imports/multiple potentially scary pop-ups) and (if the user mostly has mature cards) doesn't give easy access to testing the cards of interest (they have to filter for One idea might be to create a purely "testing" deck where all the notes would have different GUIDs, so the user could safely import, test and delete the "testing" deck. This can wait for after merging, though (these changes shouldn't be in the committed code). (It should be a matter of clearing the GUIDs in |
DocumentationI think we should keep documentation to a minimum while the deck is still experimental to avoid unnecessary PRs. Notably, I'm wondering whether the proposed changes to Very minor, but I find that some implementation details, like the IIFE, would be better off as comments in the code to avoid cluttering the main documentation. In the Map coloursOne aspect we haven't discussed yet is accessibility, and more specifically colour blindness. In fact, red/green is a well-known troublesome combination. I reckon we should keep the same red tint as the static maps for "correct answer" and find some sort of distinctive colour + pattern combination for "wrong answer". For the colour, maybe a shade of purple? For the pattern, I'm thinking maybe dots, crosses or zigzags, since diagonal bars are already used for disputed territories. Here's a tool that can generate a CSS-only zigzag pattern, for instance:
ConfigurationI agree with @aplaice about moving the user-facing config into the template. Not sure I understood the double-load issue but local storage seems like a good solution and hiding this complexity away into a Not sure about the boolean strings. After all, it doesn't really prevent users from typing Caching files in
|
- Note that this commit is atomic part of the previous one and the sole purpose of separation is readability of mainstream changes of the former
- For maximum flexibility introduced ability to disable correctly selected green region highlighting via card template - JS code moved out into separate files for front and back card templates - Each file is wrapped into IIFE to avoid re-declaring variables when switching between cards (Anki uses persistent webview)
- In case of interactive map trouble to prevent user from need to reimport normal UG deck implemented logic to make it possible to disable interactive map - In case of being disabled it gets replaced with static fallback as if the deck is of Extended type
- Foreseeing possible issues with dragging compatibility on smartphones (e.g. accidental click on the region) implemented a switch to disable automatic card turn to answer side
- Defaulting to static fallback image when card has empty "Region code" field - Switch from explicit constructors for type conversions to implicit idiomatic operator usages (e.g. +<string> to get number value)
- Prior to the change if something went wrong with script loading nothing would be shown to the user on question or answer - Described commit implements defensive tactic so that if script fails to load static fallback is shown anyway
- To maintain flexibility status the described commit implements switch to control display of interactive map on mobile agents (including AnkiDroid and AnkiMobile) - The use case of the option might be too small device dimensions or simple preference of static map when reviewing on smartphone
- Tooltip on answer card side is disabled by default as it is assumed to disrupt remembering process (e.g. first card you learn England, but select Germany and tooltip shows its name, then the next card is Germany and you will easily guess it) - Mobile hack of hidden tooltip to make dragging work is removed as it seems that some other factor caused the dragging issue
- Merge scripts for both card sides with intention to factoring out common parts and reducing code size - Improve code readability by moving out a lot of logic into separate methods, each having documentation written for it
- Move out the logic to separate isolated function - Document the specifics of the moved function - Minor unrelated code corrections
- Put all the configuration properties in common object - Shorten constant object properties names
- One of the earlier commit erased the mobile hack for dragging on mobile to work presuming that the tooltip is not the cause of the issue - The assumption was wrong and has arisen due to incorrect mobile media folder update - The described commit reverts the hack removal and integrates it with tooltip switch to work correctly
- In order to reduce symbols and complexity of code - shorthand for sessionStorage is implemented to either retrieve string or boolean value. Appropriate changes are made to switch from implicit boolean casting to the new shorthand - In order to aid readability, map initialization conditions are moved out to separate functions, each given descriptive name and documentation comment
- Prior to the change any positive integer yielded true - Described commit correct function body to correspond to the behaviour written in function documentation comment
- In order to improve fault-tolerance of the program add a logic to disable interactive map in case there is code error thrown - Note, that interactiveMapMode is first enabled for map initialization to work correctly and only if error occurs it is disabled (in contrast to enabling the mode after initialization occurs which won't work) - Resolving logic of whether map mode is enabled is moved out into separate function with readability purpose
- As experimental deck does not require much customized styling that differs from any other deck, there is no sense in storing separate stylesheet - thus the merge
- Note that non-experimental decks are not affected by the merge as:
* `@import url("_jsvectormap.min.css");` at-rule is ignored due to stylesheet _jsvectormap.min.css not existing
* Moreover, even assuming the user installs experimental deck and then downgrades to regular one, leaving _jsvectormap.min.css in their collection folder, the rules in that stylesheet are isolated and applicable to service classes assigned to the map element (which is absent on regular decks)
* `.value--map` rule is not applied as there are no elements with this class (assuming future commits will be mindful of this class already existing and won't introduce it into the decks)
- In order to increase maintainability of the code, move out all the "magic" strings being used in the code into one place
- Standard map that comes with the jvm library is losing significant land details - In order to increase UX, integrate additional map with much more land mass details - Note, that as a result of performance considerations described commit as well introduces possibility to switch to low-detail map as some users may not possess enough available computing resources, specifically smartphones. - Also note that it is conscious decision to include both map initializations in card templates as it did not show to slow performance significantly. Further commits may improve existing logic
axelboc
force-pushed
the
interactive-map
branch
from
0536256 to
a2e8d0f
Compare
|
I've rebased from the GitHub UI after merging #639, I hope it won't cause an issue. I've also edited my previous comment with an idea to solve the caching issue with files in |
|
Getting back with the subset of the agreed changes. Here is a brief summary (see commits for details): Done:
Skipped:
First of all, I would like to thank you all for your time and effort in reading, reviewing and discussing all the details. I appreciate the enormous support! Now to the comments. @aplaice:
Of course, none of the "plumbing" is included.
I suppose the plan is to gradually remove static images from experimental deck when it becomes "stable"? I may miss some details, but leaving static images as fallback in experimental deck and then "stabilizing" it (i.e. making it new extended) makes the deck everything the extended one now represents + interactive map. So why exactly having "old extended" one as a variant?
Thank you for clarifying. Now that I think of it again the proposed versioning system actually makes sense.
One other option that I used when testing on AnkiDroid is to create separate AnkiWeb account, link it to testing Anki Desktop profile and then sync AnkiDroid with the account (by force downloading the changes). Such solution is basically an ad-hoc AnkiDroid profile change. Though not sure whether possible on AnkiMobile. @axelboc:
Moving to wiki is a great solution for PRs minimization as documentation for the interactive map is expected to be updated frequently, with each incremental improvement. Also, I have quite an exhaustive smoke testing checklist, which I followed before creating PR (and then after introducing minor changes) that might be valuable to place into wiki as well (due to gradually increasing complexity of program logic the need for some validation arises).
Most definitely agree as I imagine it to be easier to have the explanation exactly in place that it clarifies.
If I understand correctly the idea is about having "teasers" of extended and experimental decks in "Features" section with wiki links for advanced details, right?
To be honest it was the reason of my initial motivation to move color properties into user configuration - so that users with special needs are able to configure the colors correspondingly (and those who want deeper customization as well).
This is brilliant idea (aside from the necessity to introduce appropriate logic into underlying library)! Maybe it is even unnecessary to deviate from current static map style - and simply use red tint + pattern combination for "wrong answer"?
That is on me, missed to provide one important detail. AnkiDroid uses separate global scopes for question and answer card sides, meaning that moving the <script>
function getUserConfig() {
return {
commonFeatures: {
interactiveEnabled: "true",
...
}
</script>
<div class="value value--top">{{Country}}</div>
<hr>
...
--
<div class="value value--top">{{Country}}</div>
...won't work as the function is not visible to the back card side global scope. Thus the notion of doubling configuration: <script>
function getUserConfig() {
...
}
</script>
<div class="value value--top">{{Country}}</div>
<hr>
...
--
<script>
function getUserConfig() {
...
}
</script>
<div class="value value--top">{{Country}}</div>
...Aside from AnkiDroid concern this solution would have been the preferred one.
As described in
Yes, it is. Will keep in mind the mentioned technique. Although the disadvantage I see is that with time and a series of updates user media folder will accumulate significant amount of older unused files (which are not removed by "Check media" option, due to those files being considered intrinsic as they are prefixed with underscore). They do not weigh much (at least now), but still. I am yet to investigate the logic of media update and possible options. Will get back with results later on.
Mostly agree with the proposed plan. Just a few questions:
I assume this step does not include the official public release with announcement? It is more like a beta-testing stage for "closely involved" only?
Same question about preservation of static images in experimental deck after making it "new extended" one as to @aplaice above. Although from what you tell I figure that the "old extended" is planned to be discontinued from releases, while leaving an instruction to generate it in case some old school folks will want to. |
|
I'm a bit late, but I'll write my opinion here regarding some topics discussed. If they are not still relevant, I'm sorry! =P
Maybe black for incorrect and red for correct. Black is usually supposed to be worse than red, and by the time the user sees an interactive map, they should have seen a static Map - Country card and maybe associate red with correct. Just an idea, but I'm not sure about this at all. :3
Black and red would work for this as well. In case of red-blindness, it'd be seen as black and some shade of grey (same color as the one seen in the Map - Country), and in case of green- or blue-blindness (or any combination) it doesn't change anything. =)
Same opinion here. I think that's the best advantage of using a VCS. ^^ Thanks for all the effort to you all! ^^ |
| sessionStorage.setItem("userConfig", | ||
| JSON.stringify({ |
There was a problem hiding this comment.
I think what @aplaice and I had in mind was to somehow hide away those two lines. You could expose a defineInteractiveMapConfig but that would mean importing yet another script at the top of the template...
<script src="_ug-interactive_map_setup.js"></script>
<script>
defineInteractiveMapConfig({
commonFeatures: { ... },
commonColors: { ... },
})
</script>// _ug-interactive_map_setup.js
(function () {
function defineInteractiveMapConfig(userConfig) {
sessionStorage.setItem("userConfig", JSON.stringify(userConfig));
}
}());Not great... An alternative might be to expose the config on the global scope:
<script>
window.INTERACTIVE_MAP_CONFIG = {
commonFeatures: { ... },
commonColors: { ... },
};
</script>// _ug-interactive_map_config.js
(function () {
// Retrieve and store user config in `sessionStorage` if present (i.e. on front side only)
if ('INTERACTIVE_MAP_CONFIG' in window) {
sessionStorage.setItem("userConfig", JSON.stringify(window.INTERACTIVE_MAP_CONFIG));
}
function getConfig() {
... // read from `sessionStorage`
}
}());Yet another option, perhaps cleaner, is to define the config as JSON directly in the template:
<script id="ugInteractiveMapConfig" type="application/json">
{
"commonFeatures": { ... },
"commonColors": { ... }
}
</script>// _ug-interactive_map_config.js
(function () {
// Retrieve and store user config in `sessionStorage` if present (i.e. on front side only)
const configElem = document.querySelector('#ugInteractiveMapConfig')
if (configElem) {
sessionStorage.setItem("userConfig", configElem.innerHTML);
}
function getConfig() {
... // read from `sessionStorage`
}
}());There was a problem hiding this comment.
I think what @aplaice and I had in mind was to somehow hide away those two lines. You could expose a defineInteractiveMapConfig but that would mean importing yet another script at the top of the template...
I had meant not including the getMapConfig, filterObj and other functions in the template and leaving them in _ug-interactive_map_config.js. It would be slightly better not to have the two lines of stringifying and setting session storage in the template, but it should be clear to both "laypeople" and programmers that they shouldn't be touched, and they're not much "weight-wise".
I do like the second approach, though (both from a "cleanness" perspective — it makes it clear that the configuration is configuration and not code — and from a "less risk of the user messing things up" one), and I think it should work on AnkiDroid since we'll still be setting the sessionStorage on the front card, just in a different place?
(But I think that the status quo is fine!)
Yes, just a simple paragraph for each, extended and experimental, explaining what's different about them, maybe with some screenshots showing the extra templates in action, like for the standard deck.
Worth a try! Same with @josealberto4444's suggestion of using black. The config will let us try different combinations easily, which is awesome.
Oh yeah, that's not great if the files start to accumulate.
Exactly, people who are subscribed to PR-related events and will know when this MVP is merged.
Exactly, it doesn't cost anything to keep the original Country - Map template along with a Brain Brew recipe. But the release notes won't include ZIP files for this "old extended" deck; only for the new, interactive one. Concerning the static images, won't they have to stay anyway for the Map - Country template? Or were you thinking of putting an interactive map on the front side of this template as well? |
helitopia
force-pushed
the
interactive-map
branch
from
d55dbb0 to
9bbde54
Compare
|
Force pushed suggested changes (instead of adding new commit with miscellaneous edits) to keep commit history cleaner.
This option appears the cleanest of the selection.
Is it worth implementing though? Hiding these two lines won't make much difference, as there are infinitely many ways to break the code, but adding this change will require additional hack in the back-end (config script): // _ug-interactive_map_config.js
(function () {
// Retrieve and store user config in `sessionStorage` if present (i.e. on front side only)
const configElem = document.querySelector('#ugInteractiveMapConfig')
if (configElem) {
sessionStorage.setItem("userConfig", configElem.innerHTML);
}
function getConfig() {
... // read from `sessionStorage`
}
}());IMO status quo is the most concise it can be. (Edit: meaning balanced amount of code in both front template and configuration script) Or there is some other significant reason to do so I am missing? |
|
Thanks everyone for the comments (sorry I was unavailable for a couple of days) and above all thanks so much @helitopia for your continued updates! IMO we should merge now to avoid bike-shedding! (@axelboc I leave the last word to you, as always, though! :)) How do we make the deck easiest for testing? Do we create a draft/beta release ("pre-release") (with just the experimental deck (?))? (The main disadvantage is excessive notification for people watching releases but not other activity. OTOH forcing people to build the deck themselves is a barrier to testing. We sort-of have a rudimentary quick-release system here with artifacts published here, but it's not in the main repo due to converns about CI having access to the "write repo" permission.) Bikeshedding below :) Colours/patterns
I fully agree with @axelboc that we should care about accessibility and all the ideas (red vs. black, red vs. zigzag purple, red vs. zigzag red) are great, but I don't think we've managed to settle on anything, so maybe let's just stick with red/green for now, with the plan that we will switch to something colour-blind-friendly in the next beta, and in the meantime we (and any other testers) can all play around with all the alternatives?
My main motivations was:
(2 is more important than 1.) (This is a decision for the future, though, and it wouldn't be just up to me to decide :)) (It also depends on how many variants we have, and how we display them in terms of releases — having them on the main release pages might end up overwhelming.) (If we end up with many variants we might want to have some sort of simple github pages webpage which would allow users to pick version, language, variant etc.)
That works, but it's not very convenient, though :(
Actually, I've just now realised that AnkiMobile (even on very old versions!) allows for multiple profiles, out-of-the-box! Sorry for not remembering earlier! Minimising config code in template
I think the key goal was making the template (as opposed to the back-end |
The only remaining thing, if that's okay, would be to move the documentation out of I've created a page and you should be able to edit it @helitopia. |
- In order to make customizable part of program configuration more user-friendly - move the mentioned configuration into note template to be able to open it from any device and easily modify the available properties
helitopia
force-pushed
the
interactive-map
branch
from
9bbde54 to
0f09943
Compare
|
Again, force pushed for the sake of clarity of commit history. Sorry if it makes review more difficult. For now linked documentation page via experimental deck mention in |
|
Excellent, thanks! |
|
I've added a page to the Wiki describing switching deck/note type: https://github.com/anki-geo/ultimate-geography/wiki/Switching-deck-type#specific-guides I hope it's clear! (The descriptive section is probably unnecessary — I've moved it to the bottom, so that people who are interested can read it, but it doesn't distract those who aren't.) I've also updated the workflow that publishes built decks based on the latest commit to also publish the experimental decks, here. (I'm not convinced that we shouldn't just do a normal draft release, though.) |
|
Fantastic work @aplaice, this will be super helpful, I think! Feel free to link to it from the |
Well done!
Is it about time to do so? Or since you have written a tabular guide for all combinations of upgrading/downgrading this would be unnecessary? @aplaice |
Temporary decks
I'm not sure. The instructions should hopefully make the process smoother, but people might still not want to play around with their main AUG deck in case they mess up the review history (if they take care, then this shouldn't be an issue, but still). Using separate profiles for testing is probably the sanest approach, but it's tricky on AnkiDroid. (It does work on both Anki desktop and AnkiMobile, though.) (On Android, you can clear everything out, sync with a different account, and then sync with your original account, but if you have a large collection that's rather slow. I believe you can also use parallel AnkiDroid builds to simulate profiles, but it's finicky.) In case we do want temporary decks, the following bash script will generate them: #!/bin/sh
# We can't do anything smarter here (we have to insert the right
# number of commas ourselves), since some guids contain commas.
sed '2,$ s/,.*/,,,,,,,,,,,,,/g' -i src/data/guid.csv
# The following is inefficient (rewriting the same file n times) but results in cleaner-looking code.
# We can't just use one uuidgen invocation with a more general regexp, because we'd have the same UUID for all 4 decks.
for uuid in 43c5ba66-9a65-11e8-90c9-a0481cc15658 cb4d32ee-12ed-9960-1841-28c09449ded0 75bfcdb5-0ff3-4038-83cb-3e6ed974f439 6c995ee1-4b62-4019-a033-de0ef8651c83
do
sed 's/crowdanki_uuid: '"$uuid"'/crowdanki_uuid: '"$(uuidgen -r)"/ -i recipes/source_to_anki_\[experimental\].yaml
done
sed -r 's/name: Ultimate Geography( \[[A-Z][A-Z]\]|)$/name: Ultimate Geography\1 experimental interactive/' -i recipes/source_to_anki_\[experimental\].yaml
pipenv run build_experimentalPatterns in regionsUnfortunately, I haven't found a simple way to add patterns (zig-zags etc.) to highlight selected regions. The generated map is an SVG so CSS applies differently to it than to normal HTML (i.e. @axelboc's site regrettably won't work out of the box). AFAICT the normal way of adding more complex styling (anything more complicated than a single colour, is by using This means that I think we'd have to inject the pattern definition into the SVG element generated by jsvectormap, (and refer to it simply via |
Thank you for the investigation! As I mentioned in the comment:
I assume that the ability to set selected region highlight to a pattern is a useful feature so it is a good idea to mainstream (or include in the fork if maintainer is not reachable) implemented functionality into jsvectormap codebase, instead of creating ad hoc solution in our codebase. But before actually getting to the proper implementation I think ad hoc solution you described will still be required first just to create a PoC of pattern highlight feature (instead of solid color) so that we are able to gauge the applicability of the idea (i.e. if it looks fine on the map). |
|
For now, I've opened a discussion here: Please let me know if something is missing/wrong/too verbose! (@axelboc/anyone else with admin rights also please feel free to edit directly.) I've arbitrarily (for ease of differentiation) named the deck v0.0. I used a "parallel" deck (with separate GUIDs/UUIDs) — it doesn't really hurt unless somebody wants to immediately upgrade to the new interactive version (and if people do want that then we should probably just do a release). We could also create a release, but maybe for the next iteration? (It seemed we couldn't really reach consensus on whether we wanted one now.) |
|
Amazing description, thank you!
By the way, I am currently working on summarizing and describing the remaining items on the agenda, so will come back with updates in the original issue soon.
Nevertheless, for now, I think it would be great to include a link (or artifact directly) to the "non-parallel" deck (with the same GUIDs/UUIDs) so that those who want can easily upgrade from the discussion page. Moreover, IMO (although I am in no way pushing it) those who upgrade are potentially more likely to discover issues / have ideas as they will consistently use the map in contrast to importing the deck separately, playing around with it for a day and forgetting about it (or removing altogether). |
Original issue
Hi @aplaice, @axelboc
Getting back with the PoC discussed in the original issue, integrated into experimental deck.
Please be sure to read added parts in
README.MDandCONTRIBUTING.MDbefore getting to review the PR as it will add clarity to the reasoning and structure of the changes. Also, most introduced changes are represented by meaningful and detailed commit messages which might help in understanding of the codebase (which might need to be squashed before the merge, it is up to you).It was a top priority to follow both explicit and implicit project conventions, while it is acknowledged that some parts might have been missed, so the feedback is much appreciated.
There is an overwhelming amount of things discovered, ideas generated, questions raised during implementation of the change. All of them will later on appear for discussion in the original issue. The goal of this PR is to focus exlcusively on existing concepts and corresponding logic leading to release of minimal viable product to get the project started.
Over the course of working on the change a number of remarkable improvements over PoC were made. In particular:
<script>elements of the note templateSome notable implementation details:
source_to_anki.yamlrecipe being already complex enough, the infrastructure part of the experimental deck creation was written as a separate configuration -source_to_anki_[experimental].yaml. The point is reinforced by the fact that the deck version numbering does not match its counterparts and persistent existence of the deck being under question altogether@importat-rule for underlying library is ignored when non-experimental deck makes use of the stylesheet, so there should be no issue with it. Although now writing the description I understand that I could make use of<style>element to import the stylesheet directly in the note template instead of using the at-rule - and probably will do so eventually (either with next release or with corrections of this PR, if there will be any)